Skip to content

Fix tree view checkbox selection for backup dialog. #9649#9671

Open
RohitBhati8269 wants to merge 2 commits intopgadmin-org:masterfrom
RohitBhati8269:GH-9649
Open

Fix tree view checkbox selection for backup dialog. #9649#9671
RohitBhati8269 wants to merge 2 commits intopgadmin-org:masterfrom
RohitBhati8269:GH-9649

Conversation

@RohitBhati8269
Copy link
Contributor

@RohitBhati8269 RohitBhati8269 commented Feb 24, 2026

  • Modified toggleCheck to collect all checked/indeterminate nodes from the
    entire tree, not just the clicked node's subtree
  • Fixed ancestor state calculation to properly distinguish between fully
    checked (state === true) and indeterminate states
  • Added the isIndeterminate flag to node data to help the backup dialog differentiate
    between full schema selection and partial table selection
  • Added traversal logic to find root nodes and iterate through all siblings
    to collect a complete selection state

Summary by CodeRabbit

  • Bug Fixes
    • Improved tree selection so toggling an item updates its descendants and parents correctly; callbacks now receive a complete snapshot of checked and partially-selected items.
  • Chores
    • Minor internal cleanup in the backup UI code for clearer handling of selection state.

@coderabbitai
Copy link

coderabbitai bot commented Feb 24, 2026

Walkthrough

Replaces PgTreeView's selection callback to update the clicked node, its descendants, and ancestor indeterminate states, then collects all checked and indeterminate nodes and passes them (with isIndeterminate flags) to the existing selectionChange callback. Also adapts backup UI to destructure the new node wrapper.

Changes

Cohort / File(s) Summary
Tree Selection State Logic
web/pgadmin/static/js/PgTreeView/index.jsx
Reworked checkbox toggle flow: toggles clicked node and descendants, recomputes ancestor checked/indeterminate states, then traverses tree to collect and forward all checked and indeterminate nodes as wrapper objects ({ node, isIndeterminate }) to selectionChange.
Backup UI consumer update
web/pgadmin/tools/backup/static/js/backup.ui.js
Updated iteration to destructure incoming selection wrappers: state?.objects?.forEach(({ node, isIndeterminate }) => ...) and adjusted conditional checks to use the isIndeterminate flag directly.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title directly addresses the main change: fixing tree view checkbox selection for the backup dialog, which is the primary objective across both modified files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@web/pgadmin/static/js/PgTreeView/index.jsx`:
- Line 91: Change the concise-arrow forEach callbacks to block-bodied arrows
that explicitly perform their side effects and return nothing; e.g., replace the
single-expression callback in the n.children?.forEach(child =>
updateDescendants(child, val)); call with a block-bodied callback that calls
updateDescendants(child, val) inside braces, do the same for the forEach inside
collectAllCheckedNodes (which currently has a bare return;) and the third
flagged forEach at line 146 so none of the forEach callbacks have implicit
returns; this will satisfy the lint rule and keep intent explicit.
- Around line 126-131: The code mutates the parent-owned node data by setting
n.data.isIndeterminate, causing stale flags; instead stop mutating n.data and
create a wrapper object for each selected node (e.g., push { node: n,
isIndeterminate: state === 'indeterminate' } into allCheckedNodes) so
isIndeterminate is derived from checkedState/newState rather than stored on
n.data; update the selectionChange call-site and the backup-dialog consumer to
read isIndeterminate from the wrapper object (or the wrapper's property) instead
of node.data.isIndeterminate so the flag is always correct and never mutates the
original data.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 97c90f1 and 3969615.

📒 Files selected for processing (1)
  • web/pgadmin/static/js/PgTreeView/index.jsx

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
web/pgadmin/static/js/PgTreeView/index.jsx (1)

233-238: ⚠️ Potential issue | 🟡 Minor

PropTypes are out of sync with actual props.

DefaultNode receives isChecked and onToggle props (lines 198, 201) but PropTypes lists tree and onNodeSelectionChange instead.

🔧 Proposed fix
 DefaultNode.propTypes = {
   node: PropTypes.object,
   style: PropTypes.any,
-  tree: PropTypes.object,
   hasCheckbox: PropTypes.bool,
-  onNodeSelectionChange: PropTypes.func
+  isChecked: PropTypes.oneOfType([PropTypes.bool, PropTypes.string]),
+  onToggle: PropTypes.func
 };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/pgadmin/static/js/PgTreeView/index.jsx` around lines 233 - 238, The
DefaultNode PropTypes are incorrect: replace the obsolete props `tree` and
`onNodeSelectionChange` with the actual props used by the component (`isChecked`
and `onToggle`). Update DefaultNode.propTypes so it declares node:
PropTypes.object, style: PropTypes.any, hasCheckbox: PropTypes.bool, isChecked:
PropTypes.bool, and onToggle: PropTypes.func (remove `tree` and
`onNodeSelectionChange`) to match the usages in DefaultNode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@web/pgadmin/static/js/PgTreeView/index.jsx`:
- Around line 233-238: The DefaultNode PropTypes are incorrect: replace the
obsolete props `tree` and `onNodeSelectionChange` with the actual props used by
the component (`isChecked` and `onToggle`). Update DefaultNode.propTypes so it
declares node: PropTypes.object, style: PropTypes.any, hasCheckbox:
PropTypes.bool, isChecked: PropTypes.bool, and onToggle: PropTypes.func (remove
`tree` and `onNodeSelectionChange`) to match the usages in DefaultNode.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3969615 and a3f1c05.

📒 Files selected for processing (2)
  • web/pgadmin/static/js/PgTreeView/index.jsx
  • web/pgadmin/tools/backup/static/js/backup.ui.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant